Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bootloader: Add a zipl bootloader backend #1937

Merged
merged 1 commit into from
Oct 16, 2019
Merged

bootloader: Add a zipl bootloader backend #1937

merged 1 commit into from
Oct 16, 2019

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Oct 14, 2019 •
edited by tuan-hoang1
Loading

zipl is a bit special in that it parses the BLS config files
directly but we need to run the command to update the "boot block".

Hence, we're not generating a separate config file like the other
backends. Instead, extend the bootloader interface with a post_bls_sync
method that is run in the same place we swap the boot/loader symlink.

We write a "stamp file" in /boot that says we need to run this command.
The reason we use stamp file is to prevent the case where the system is
interrupted after BLS file is updated, but before zipl is triggered,
then zipl boot records are not updated.
This opens the door to making things eventually-consistent/reconcilable
by later adding a systemd unit to run zipl if we're interrupted via
a systemd unit - I think we should eventually take this approach
everywhere rather than requiring /boot/loader to be a symlink.

Author: Colin Walters [email protected]
Tested-by: Tuan Hoang [email protected]
Co-Authored-By: Tuan Hoang [email protected]

@cgwalters
Copy link
Member Author

(Only compile tested!)

@cgwalters cgwalters mentioned this pull request Oct 14, 2019
Merged
@Prashanth684
Copy link

(Only compile tested!)

i'd like to test these changes locally as part of creating an fcos/rhcos image on s390x. any idea how i can do that ?

@cgwalters
Copy link
Member Author

i'd like to test these changes locally as part of creating an fcos/rhcos image on s390x. any idea how i can do that ?

See https://github.com/coreos/coreos-assembler/blob/master/README-devel.md#using-overrides

@vtolstov
Copy link

can you share link to this kind of bootloader - zipl ?

@jlebon jlebon mentioned this pull request Oct 15, 2019
Merged
@cgwalters
Copy link
Member Author

can you share link to this kind of bootloader - zipl ?

See https://github.com/ibm-s390-tools/s390-tools/ for https://en.wikipedia.org/wiki/Linux_on_z_Systems

@tuan-hoang1
Copy link
Collaborator

tuan-hoang1 commented Oct 15, 2019 •
edited
Loading

Given we make *out_is_active = FALSE; and specifically set sysroot.bootloader zipl in create_disk.sh, I think we still need this, right ?

diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c
index b654aff2..94116290 100644
--- a/src/libostree/ostree-repo.c
+++ b/src/libostree/ostree-repo.c
@@ -3185,7 +3185,7 @@ reload_sysroot_config (OstreeRepo          *self,
      * https://github.com/ostreedev/ostree/issues/1719
      * https://github.com/ostreedev/ostree/issues/1801
      */
-    if (!(g_str_equal (bootloader, "auto") || g_str_equal (bootloader, "none")))
+    if (!(g_str_equal (bootloader, "auto") || g_str_equal (bootloader, "none") || g_str_equal (bootloader, "zipl")))
       return glnx_throw (error, "Invalid bootloader configuration: '%s'", bootloader);
 
     g_free (self->bootloader);

I thought "auto" should cover when we specifically set bootloader, but seems not likely as rpm-ostreed reported
Oct 15 16:40:28 coreos rpm-ostree[1360]: error: Couldn't start daemon: Error setting up sysroot: Invalid bootloader configuration: 'zipl'

@cgwalters
Copy link
Member Author 

if (!(g_str_equal (bootloader, "auto") || g_str_equal (bootloader, "none") || g_str_equal (bootloader, "zipl")))

Oh right. Thanks, I merged that bit and added the other bootloaders too for completeness. This bit is ugly...need to rewrite the bootloader stuff, but let's do the easy change for now.

@tuan-hoang1 tuan-hoang1 force-pushed the zipl-backend branch 2 times, most recently from 0900648 to c60ed2f Compare October 16, 2019 10:07
@tuan-hoang1
Copy link
Collaborator

To summarize:
Colin's 542f550 commit : add a "zipl" check in libostree/ostree-repo.c
Colin's 5646da4 commit : add _ostree_bootloader_zipl_new() to create bootloader object for zipl case.
My last 2 commits : add a comment to make it clearer for why we do _ostree_bootloader_zipl_new() instead inside libostree/ostree-sysroot-deploy.c. Also update git commit message with some more details.

Colin @cgwalters, thanks a lot for quick turnaround ! This works perfectly well. I think we are good to go now.

@tuan-hoang1 tuan-hoang1 changed the title WIP: Add a zipl bootloader backend bootloader: Add a zipl bootloader backend Oct 16, 2019
@tuan-hoang1
Copy link
Collaborator

Looks like there was some permission issue in ostree source tree ? I pulled your remote tree and make a branch on my local repo, maybe that's an issue ? Let me clone your tree then ...

@cgwalters
Copy link
Member Author

Repushed to retrigger CI.

jlebon
jlebon reviewed Oct 16, 2019
View reviewed changes
src/libostree/ostree-repo.c Outdated Show resolved Hide resolved
@cgwalters @tuan-hoang1
bootloader: Add a zipl bootloader backend
c61234a 
zipl is a bit special in that it parses the BLS config files
directly *but* we need to run the command to update the "boot block".

Hence, we're not generating a separate config file like the other
backends.  Instead, extend the bootloader interface with a `post_bls_sync`
method that is run in the same place we swap the `boot/loader` symlink.

We write a "stamp file" in `/boot` that says we need to run this command.
The reason we use stamp file is to prevent the case where the system is
interrupted after BLS file is updated, but before zipl is triggered,
then zipl boot records are not updated.
This opens the door to making things eventually-consistent/reconcilable
by later adding a systemd unit to run `zipl` if we're interrupted via
a systemd unit - I think we should eventually take this approach
everywhere rather than requiring `/boot/loader` to be a symlink.

Author: Colin Walters <[email protected]>
Tested-by: Tuan Hoang <[email protected]>
Co-Authored-By: Tuan Hoang <[email protected]>
@jlebon
Copy link
Member

jlebon commented Oct 16, 2019

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cgwalters
Copy link
Member Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 38729a4 into ostreedev:master Oct 16, 2019
@tuan-hoang1 tuan-hoang1 mentioned this pull request Oct 18, 2019
Closed
33 tasks
@wmanley wmanley mentioned this pull request Oct 26, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlebon jlebon jlebon left review comments

Assignees

@jlebon jlebon

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cgwalters @Prashanth684 @vtolstov @tuan-hoang1 @jlebon @openshift-ci-robot @openshift-merge-robot